Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Personal settings auth tokens #24703

Merged
merged 3 commits into from
May 23, 2016
Merged

Personal settings auth tokens #24703

merged 3 commits into from
May 23, 2016

Conversation

ChristophWurst
Copy link
Contributor

@ChristophWurst ChristophWurst commented May 18, 2016

requires #24686, #24677
fixes #23458

TODO:

Ideas for follow-up PRs:

  • Allow omitting the dashes of device tokens, so users can use both 'ABCDEFGHIJKLMNOPQRST' and 'ABCDE-FGHIJ-KLMNO-PQRST'
  • Don't allow the users to kill the active browser session as that would log them out (probably unintended)
  • Convert sync client user agents like Mozilla/5.0 (Linux) mirall/2.1.1 to something like ownCloud sync client 2.1.1

cc @MorrisJobke

@ChristophWurst ChristophWurst added this to the 9.1-current milestone May 18, 2016
@ChristophWurst ChristophWurst changed the title Personal settings auth tokens [WIP] Personal settings auth tokens May 18, 2016
@@ -837,6 +837,13 @@ public function getUserSession() {
}

/**
* @return Authentication\Token\DefaultTokenProvider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either use the full class or import the full class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also only public interfaces shall be returned™

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no public interface yet, because it's not yet done add a FIXME/TODO to make that clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is an internal only method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then @internal ? ;)

@MorrisJobke
Copy link
Contributor

After applying this:

diff --git a/settings/css/settings.css b/settings/css/settings.css
index be61265..42e2594 100644
--- a/settings/css/settings.css
+++ b/settings/css/settings.css
@@ -100,21 +100,27 @@ input#identity {
 table.nostyle label { margin-right: 2em; }
 table.nostyle td { padding: 0.2em 0; }

-#sessions,
-#devices {
-   min-height: 180px;
-}
 #sessions table,
 #devices table {
    width: 100%;
-   min-height: 150px;
-   padding-top: 25px;
 }
 #sessions table th,
 #devices table th {
    font-weight: 800;
 }

+#sessions table th,
+#sessions table td,
+#devices table th,
+#devices table td {
+   padding: 10px;
+}
+
+#sessions .token-list td,
+#devices .token-list td {
+   border-top: 1px solid #DDD;
+}
+
 /* USERS */
 #newgroup-init a span { margin-left: 20px; }
 #newgroup-init a span:before {
diff --git a/settings/js/authtoken_view.js b/settings/js/authtoken_view.js
index 0ca1682..b2bdee8 100644
--- a/settings/js/authtoken_view.js
+++ b/settings/js/authtoken_view.js
@@ -50,7 +50,9 @@
            list.html('');

            tokens.forEach(function(token) {
-               var html = _this.template(token.toJSON());
+               var t = token.toJSON();
+               t.lastActivity = moment(t.lastActivity, 'X').format('LLL');
+               var html = _this.template(t);
                list.append(html);
            });
        },

It looks like this:

bildschirmfoto 2016-05-18 um 17 45 17

@MorrisJobke
Copy link
Contributor

@ChristophWurst @PVince81 What is the best way in Backbone to apply the date format stuff? Should this be a template filter?

@PVince81
Copy link
Contributor

So far I'd just preformat the date before passing it to the template as string.
See how it's done in here: https://github.com/owncloud/core/blob/master/apps/files/js/mainfileinfodetailview.js#L148

@@ -36,7 +36,8 @@
$application->registerRoutes($this, [
'resources' => [
'groups' => ['url' => '/settings/users/groups'],
'users' => ['url' => '/settings/users/users']
'users' => ['url' => '/settings/users/users'],
'AuthSettings' => ['url' => '/settings/personal/authtokens'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want this information to be available on the client one day, it needs to be OCS, but I think for now it's okay

@ChristophWurst ChristophWurst force-pushed the personal-settings-auth-tokens branch 6 times, most recently from fef6e35 to 4fae242 Compare May 19, 2016 10:19
@ChristophWurst
Copy link
Contributor Author

bildschirmfoto von 2016-05-19 11-19-36
bildschirmfoto von 2016-05-19 11-19-48

(Note that the token is now 4x5chars and not 5x5 like in the screenshot)

@ChristophWurst ChristophWurst changed the title [WIP] Personal settings auth tokens Personal settings auth tokens May 19, 2016
@ChristophWurst
Copy link
Contributor Author

@MorrisJobke @LukasReschke @rullzer review please

@PVince81
Copy link
Contributor

PVince81 commented May 20, 2016

  • BUG: use $(...).tooltip() on the trashbin element to make the tooltip look better

@PVince81
Copy link
Contributor

PVince81 commented May 20, 2016

@PVince81
Copy link
Contributor

I'd even say, as soon as the token field gets displayed, autoselect it. If the user clicks away and clicks on it again, reselect again. Goal is to be able to quickly Ctrl+C.

@PVince81
Copy link
Contributor

PVince81 commented May 20, 2016

  • do we want the date in the table to be human readable, like "1 hour ago" + tooltip with full date ? Use OC.Util.relativeModifiedDate(timeStamp) for that

var SubView = Backbone.View.extend({
collection: null,
type: 0,
template: Handlebars.compile(TEMPLATE_TOKEN),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

#device-new-token {
width: 186px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone knows a better way let me know

@PVince81
Copy link
Contributor

PVince81 commented May 20, 2016

  • minor: missing empty message for the lists when empty. I'd suggest to hide the table header and the message "You've linked these devices" when the list is empty. Challenge is to make the list appear as soon as the first entry gets added

@PVince81
Copy link
Contributor

  • bug? I can create several devices with the same name

@ChristophWurst
Copy link
Contributor Author

ChristophWurst commented May 20, 2016

I think i broke something, no browser sessions are listed now fixed

@ChristophWurst ChristophWurst force-pushed the personal-settings-auth-tokens branch 2 times, most recently from 5895033 to 74277c2 Compare May 23, 2016 07:10
@ChristophWurst
Copy link
Contributor Author

bug? I can create several devices with the same name

Not a bug in my opinion, but I'll open an enhancement issue.

@PVince81
Copy link
Contributor

Fair enough 👍

@PVince81
Copy link
Contributor

Second review ? @nickvergessen @schiesbn @MorrisJobke @LukasReschke @Xenopathic @rullzer

$qb = $this->db->getQueryBuilder();
$qb->delete('authtoken')
->where($qb->expr()->eq('id', $qb->createNamedParameter($id)))
->andWhere($qb->expr()->eq('uid', $qb->createNamedParameter($user->getUID())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required, because id is unique?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or don't you check ownership before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen
Copy link
Contributor

Works, and I agree that the same name is "not a bug"

👍

@PVince81 PVince81 merged commit 57525a0 into master May 23, 2016
@PVince81 PVince81 deleted the personal-settings-auth-tokens branch May 23, 2016 12:17
@oparoz
Copy link
Contributor

oparoz commented May 31, 2016

Cool feature, thanks guys :)

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9.1: Pluggable Auth
5 participants